Skip to content

Conversation

tiginamaria
Copy link
Contributor

Introduce a ServerSession to manage multiple connections to the mcp server. Protocall logic of the Server was moved to ServerSession, while MCP resources logic remained in Server

Motivation and Context

  1. Fix the multiple connection issues reported for sse transport
  2. Avoid server duplications during configurations, now every connection creates a session, but the server stays single

How Has This Been Tested?

Added SseIntegrationTest and WebSocketIntegrationTest integration tests to check connection with single and multiple clients

Breaking Changes

This change caused several depreciations:

  1. The Server -> Protocol inheritance (which caused single connection issue) should be removed in future releases, now, all the methods inherited from Protocall marked as Deprecated with a Warning
  2. Websocket ktor server initialization changes in order not to create server instance per connection

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@tiginamaria tiginamaria requested a review from devcrocod July 25, 2025 17:31
var session: ServerSession? = null
try {
session = server.connectSession(transport)
awaitCancellation()
Copy link
Contributor Author

@tiginamaria tiginamaria Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finishes without await in tests, not sure if it's a right approach to fix it, please take a look here! Really need some help here

@tiginamaria tiginamaria force-pushed the tigina/server-session branch from f33add5 to 0265956 Compare July 25, 2025 20:48
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me overall!

Please take a look at my comments.
My main concern is about the behavior of the Server
It would also be great to update the existing samples and the README accordingly

@devcrocod devcrocod requested review from e5l, sdubov and Copilot July 27, 2025 15:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces server sessions to manage multiple connections to MCP servers. The implementation moves protocol logic from the Server class to a new ServerSession class, allowing the server to handle multiple simultaneous connections while maintaining a single server instance.

  • Introduces ServerSession class for managing individual client connections
  • Moves protocol-level functionality from Server to ServerSession
  • Updates WebSocket and SSE transport implementations to support multiple sessions
  • Adds comprehensive integration tests for both single and multiple connection scenarios

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
SseIntegrationTest.kt New integration test verifying SSE transport with single and multiple client connections
WebSocketIntegrationTest.kt New integration test verifying WebSocket transport with single and multiple client connections
SseTransportTest.kt Updates to use new SseTransportManager instead of direct transport map
WebSocketMcpTransport.kt Adds debug logging for WebSocket transport operations
Protocol.kt Refactors logger variable naming for consistency
WebSocketMcpServerTransport.kt Adds debug logging for session header validation
WebSocketMcpKtorServerExtensions.kt Major refactor to support session-based architecture with new API and deprecation warnings
ServerSession.kt New class implementing session-specific protocol handling moved from Server
Server.kt Refactored to manage multiple sessions while maintaining resource management
SSEServerTransport.kt Removes unused import
KtorServer.kt Introduces SseTransportManager for better session management
WebSocketMcpKtorClientExtensions.kt Adds debug logging for client connection process
WebSocketClientTransport.kt Adds debug logging for session initialization
SSEClientTransport.kt Adds debug logging for message sending and endpoint connection
Client.kt Improves error handling during connection initialization
kotlin-sdk.api Updates API surface to include new classes and deprecation warnings
Comments suppressed due to low confidence (1)

src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt:37

  • The test method name 'client should be able to connect to websocket server 2' is unclear. The '2' suffix doesn't indicate what distinguishes this test from others. Consider renaming to something more descriptive like 'client establishes websocket connection successfully'.
    fun `client should be able to connect to websocket server 2`() = runTest {

Copy link
Contributor

@e5l e5l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @tiginamaria, thank you for the PR. Could you add a sample as a docs page illustrating what the difference will be between kotlin and the original TypeScript API?

@tiginamaria tiginamaria force-pushed the tigina/server-session branch from 560c13f to f9f1a15 Compare August 4, 2025 15:58
@tiginamaria tiginamaria force-pushed the tigina/server-session branch from fcf25b6 to 18a4174 Compare August 20, 2025 09:33
@tiginamaria tiginamaria requested review from devcrocod and e5l August 21, 2025 12:22
@tiginamaria tiginamaria marked this pull request as ready for review August 25, 2025 14:26
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@devcrocod devcrocod force-pushed the tigina/server-session branch from e26cea1 to 239fc97 Compare August 25, 2025 18:27
Copy link

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @tiginamaria, @devcrocod. Callback chaining implementation is very simple and it can be improved in a separate PR along with the tests.

Comment on lines +42 to +43
withContext(Dispatchers.Default) {
withTimeout(1000) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be replaced with runTest(timeout=5.seconds){ ... }?

Suggested change
withContext(Dispatchers.Default) {
withTimeout(1000) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but why exactly 5 seconds?

// }
//
// start()
// }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to remove commented code?

@@ -36,6 +39,8 @@ public suspend fun HttpClient.mcpWebSocket(
version = LIB_VERSION,
),
)
logger.debug { "Client started to connect to server" }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should it be trace level? It might be too verbose for debug.

@@ -53,7 +58,8 @@ public abstract class WebSocketMcpTransport : AbstractTransport() {
while (true) {
val message = try {
session.incoming.receive()
} catch (_: ClosedReceiveChannelException) {
} catch (e: ClosedReceiveChannelException) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e is actually not used. Can be reverted to _

* @return The result of the ping request.
* @throws IllegalStateException If for some reason the method is not supported or the connection is closed.
*/
public suspend fun ping(): EmptyRequestResult = request<EmptyRequestResult>(PingRequest())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: all explicit type arguments can be removed

Suggested change
public suspend fun ping(): EmptyRequestResult = request<EmptyRequestResult>(PingRequest())
public suspend fun ping(): EmptyRequestResult = request<>(PingRequest())

@devcrocod
Copy link
Contributor

devcrocod commented Aug 26, 2025

@tiginamaria
please check tests failed:

io.modelcontextprotocol.kotlin.sdk.integration.SseIntegrationTest.client should be able to connect to sse server[js, node] FAILED
    IllegalStateException at /mnt/agent/work/8d547b974a7be21f/ktor-server/ktor-server-cio/jsAndWasmShared/src/io/ktor/server/cio/internal/CoroutineUtils.jsAndWasmShared.kt:14

io.modelcontextprotocol.kotlin.sdk.integration.SseIntegrationTest.single sse connection[js, node] FAILED
    IllegalStateException at /mnt/agent/work/8d547b974a7be21f/ktor-server/ktor-server-cio/jsAndWasmShared/src/io/ktor/server/cio/internal/CoroutineUtils.jsAndWasmShared.kt:14

io.modelcontextprotocol.kotlin.sdk.integration.SseIntegrationTest.multiple sse connections[js, node] FAILED
    IllegalStateException at /mnt/agent/work/8d547b974a7be21f/ktor-server/ktor-server-cio/jsAndWasmShared/src/io/ktor/server/cio/internal/CoroutineUtils.jsAndWasmShared.kt:14

io.modelcontextprotocol.kotlin.sdk.integration.WebSocketIntegrationTest.client should be able to connect to websocket server 2[js, node] FAILED
    IllegalStateException at /mnt/agent/work/8d547b974a7be21f/ktor-server/ktor-server-cio/jsAndWasmShared/src/io/ktor/server/cio/internal/CoroutineUtils.jsAndWasmShared.kt:14

io.modelcontextprotocol.kotlin.sdk.integration.WebSocketIntegrationTest.single websocket connection[js, node] FAILED
    IllegalStateException at /mnt/agent/work/8d547b974a7be21f/ktor-server/ktor-server-cio/jsAndWasmShared/src/io/ktor/server/cio/internal/CoroutineUtils.jsAndWasmShared.kt:14

io.modelcontextprotocol.kotlin.sdk.integration.WebSocketIntegrationTest.multiple websocket connections[js, node] FAILED
    IllegalStateException at /mnt/agent/work/8d547b974a7be21f/ktor-server/ktor-server-cio/jsAndWasmShared/src/io/ktor/server/cio/internal/CoroutineUtils.jsAndWasmShared.kt:14

runBlocking is not supported for wasm

@devcrocod
Copy link
Contributor

I found the problem: these tests use stop for the Ktor server, but it should be stopSuspend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants